-
Notifications
You must be signed in to change notification settings - Fork 69
Support multiple groups and pipelines #1823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1823 +/- ##
==========================================
- Coverage 84.73% 84.66% -0.08%
==========================================
Files 499 499
Lines 146658 146784 +126
==========================================
- Hits 124276 124269 -7
- Misses 21848 21981 +133
Partials 534 534
🚀 New features to boost your workflow:
|
|
Thanks for the PR. Minor question on
|
| /// Supports: | ||
| /// - JSON files: `.json` | ||
| /// - YAML files: `.yaml`, `.yml` | ||
| pub fn from_file<P: AsRef<Path>>(path: P) -> Result<Self, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, maybe we'd want to consider the config crate for this? https://crates.io/crates/config . It might reduce some of the boilerplate loading config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is indeed this repetition issue, but at the same time there is also this error reporting problem described here: #1832. I need to test this crate config and other approaches to see what will help resolve a number of issues around configuration parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. One thing that's nice about this config crate is that it's possible to override the config file with environment variables:
https://docs.rs/config/0.15.19/config/#example
This can be helpful for users who have some dynamic config, but don't want to set up the machinery to deploy a templated config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good. Whatever we do, let's not invent a crate for configuration 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I will check this crate and validate the quality of the error messages
lalitb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the PR. Had a nit question, not directly related to this PR changes.
This part of the system still needs improvement. I would like to reach a situation where:
I created the following GH issue for tracking: #1837 |
jmacd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: It's not obvious from the PR how the distinction between single-pipeline configuration and pipeline-group configuration is done. I see the EngineConfig type already existed, so I suspect that's making it hard to see how it all connects.
| /// Supports: | ||
| /// - JSON files: `.json` | ||
| /// - YAML files: `.yaml`, `.yml` | ||
| pub fn from_file<P: AsRef<Path>>(path: P) -> Result<Self, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good. Whatever we do, let's not invent a crate for configuration 😂
# Conflicts: # rust/otap-dataflow/crates/controller/src/lib.rs # rust/otap-dataflow/crates/state/src/store.rs
For now, I have only added an extra parameter to the command line ( |
448d7ac
Closes #1812
This PR adds support for multiple pipeline groups and multiple pipelines per group. It is still possible to start the engine with a single pipeline, but it is now also possible to start the engine with a configuration file describing one or more pipeline groups. An example configuration is included in this PR.
The controller documentation has been updated to provide more details. Optimal placement of the ITS and pipelines per NUMA node is not yet implemented. Named channels are not implemented yet.
I did not observe any performance regression with this PR.